Skip to content

Implement selective and in-place freeze (#1063)#1102

Open
spoorcc wants to merge 9 commits intomainfrom
claude/implement-issue-1063-5NU0T
Open

Implement selective and in-place freeze (#1063)#1102
spoorcc wants to merge 9 commits intomainfrom
claude/implement-issue-1063-5NU0T

Conversation

@spoorcc
Copy link
Copy Markdown
Contributor

@spoorcc spoorcc commented Apr 2, 2026

  • Add optional project name arguments to dfetch freeze so users can
    freeze individual projects rather than all at once.
  • When the manifest lives inside a git or SVN superproject, freeze now
    edits the manifest file in-place (preserving comments, blank lines and
    indentation) instead of creating a .backup copy and regenerating from
    scratch.
  • Add update_project_in_manifest_file and helper functions in
    dfetch/manifest/manifest.py for in-place YAML text editing.
  • Add BDD feature files: freeze-specific-projects.feature and
    freeze-inplace.feature, plus the required step definitions.
  • Add unit tests for all new in-place editing helpers.
  • Add CHANGELOG entry for v0.14.0 (unreleased).

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA

Summary by CodeRabbit

  • New Features

    • dfetch freeze accepts optional project names to freeze only selected projects.
    • When run inside a git/SVN superproject, freeze updates the manifest in-place—preserving comments, layout, inline comments and line endings—and updates only the targeted project entries (no backup created).
  • Tests

    • Added feature and unit tests covering selective freezing, in-place updates, and YAML-preservation behaviors.

claude added 4 commits April 2, 2026 19:30
- Add optional project name arguments to `dfetch freeze` so users can
  freeze individual projects rather than all at once.
- When the manifest lives inside a git or SVN superproject, freeze now
  edits the manifest file in-place (preserving comments, blank lines and
  indentation) instead of creating a `.backup` copy and regenerating from
  scratch.
- Add `update_project_in_manifest_file` and helper functions in
  `dfetch/manifest/manifest.py` for in-place YAML text editing.
- Add BDD feature files: `freeze-specific-projects.feature` and
  `freeze-inplace.feature`, plus the required step definitions.
- Add unit tests for all new in-place editing helpers.
- Add CHANGELOG entry for v0.14.0 (unreleased).

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Split _set_integrity_hash_in_block (CC 11) and
_update_project_version_in_text (CC 13) into smaller helpers so every
new function stays below CC 9:

- _update_hash_in_existing_integrity (CC 6)
- _append_integrity_block (CC 3)
- _set_integrity_hash_in_block (CC 4)
- _collect_version_fields (CC 4)
- _apply_block_updates (CC 7)
- _update_project_version_in_text (CC 2)

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Both find_name_in_manifest (used by SARIF/Jenkins/CodeClimate reporters)
and _find_project_block (used by in-place freeze) independently scanned
for the '- name: <project>' line with slightly different regexes.

Extract _locate_project_name_line(lines, project_name) -> tuple | None
that performs the scan once and returns all four pieces of information
both callers need:
  - line_idx (0-based)
  - item_indent (column of the '-')
  - name_col_start / name_col_end (for ManifestEntryLocation)

find_name_in_manifest now calls _locate_project_name_line, preserving
its public interface (same error message, same ManifestEntryLocation
values including the existing "no spaces" edge case).
_find_project_block delegates the name scan to the helper and only
retains the block-boundary search loop.

Side-effect: CC of find_name_in_manifest drops 5→3, _find_project_block
drops 7→4.

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Replace specific-purpose helpers with three building-block functions:
- `_find_field`: locate a named field at exact indent within a block
- `_update_value`: replace the value on a known line index
- `_append_field`: insert a new key-value line at a given position

All higher-level helpers (_set_simple_field_in_block,
_update_hash_in_existing_integrity, _append_integrity_block,
_set_integrity_hash_in_block, _apply_block_updates) are refactored to
compose these primitives instead of duplicating regex/index logic.

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Apr 2, 2026

Note

Reviews paused

It looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the reviews.auto_review.auto_pause_after_reviewed_commits setting.

Use the following commands to manage reviews:

  • @coderabbitai resume to resume automatic reviews.
  • @coderabbitai review to trigger a single review.

Use the checkboxes below for quick actions:

  • ▶️ Resume reviews
  • 🔍 Trigger review

Walkthrough

Adds selective-project freezing (positional args to dfetch freeze) and implements in-place manifest edits for VCS superprojects while preserving comments, formatting, and EOL; non-VCS superprojects retain the backup+regenerate flow. New line-oriented YAML utilities and tests support the in-place edits.

Changes

Cohort / File(s) Summary
Changelog
CHANGELOG.rst
Adds unreleased Release 0.14.0 notes documenting selective freezing and in-place manifest edits.
CLI / Freeze command
dfetch/commands/freeze.py
Adds positional projects argument (nargs="*"), iterates manifest.selected_projects(args.projects), updates manifest in-place per project for VCS superprojects, retains backup+regenerate for NoVcsSuperProject.
Manifest in-place editing
dfetch/manifest/manifest.py
Adds update_project_in_manifest_file() and many private helpers to locate project blocks and perform line-based updates/appends for fields (revision/integrity); refactors name-location logic to _locate_project_name_line.
YAML text utilities
dfetch/util/yaml.py
New line-oriented YAML helpers: yaml_scalar, find_field, update_value, append_field to serialize scalars and safely edit YAML lines while preserving indentation, inline comments, and EOL style.
BDD feature specs
features/freeze-inplace.feature, features/freeze-specific-projects.feature
Adds feature scenarios asserting in-place manifest replacement for VCS superprojects and freezing specific projects.
Behave steps & tests
features/steps/manifest_steps.py, tests/test_manifest.py
Adds step assertions (file replaced / file absent) and extensive unit tests for locating project blocks, field find/update/append, integrity handling, EOL preservation, and end-to-end manifest text updates.

Sequence Diagram(s)

sequenceDiagram
  participant User
  participant CLI as "dfetch CLI"
  participant SP as "SuperProject (git/svn/NoVcs)"
  participant MF as "Manifest file (dfetch.yaml)"

  User->>CLI: run `dfetch freeze [projects...]`
  CLI->>SP: open/create superproject
  SP->>SP: determine superproject VCS type
  alt VCS superproject
    loop for each selected project
      SP->>MF: update_project_in_manifest_file(project, path)
      MF-->>SP: write updated manifest in-place
    end
  else NoVcsSuperProject
    SP->>MF: move `dfetch.yaml` -> `dfetch.yaml.backup`
    SP->>MF: regenerate and dump new manifest
    MF-->>SP: write new manifest + backup
  end
  CLI-->>User: exit status / output
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Possibly related issues

Possibly related PRs

Suggested labels

enhancement

Suggested reviewers

  • ben-edna
🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 59.38% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Implement selective and in-place freeze' directly and accurately summarizes the main changes: enabling selective freezing of specific projects and in-place manifest editing for VCS superprojects.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/implement-issue-1063-5NU0T

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 681-748: The current logic only adds/updates version fields but
never removes stale ones; update _apply_block_updates (used by
_update_project_version_in_text) to remove any existing "revision", "tag", and
"branch" lines from the block when they are not present in fields_to_set, and to
remove the integrity.hash entry when integrity_hash is empty (adjust or extend
_set_integrity_hash_in_block or add a _remove_integrity_hash_in_block helper to
perform the deletion). Locate and change behavior around _find_field,
_update_value, and the integrity handling so that missing keys in the project
YAML result in the corresponding lines being deleted from the block before
inserting/updating remaining fields.
- Around line 517-569: The code currently matches the first bare "- name:
<project>" anywhere in the file and doesn't accept quoted YAML scalars; update
_find_project_block and _locate_project_name_line so they only search inside the
"projects:" list and accept name values quoted with single or double quotes.
Concretely: first scan for the "projects:" key and compute its block
range/indentation, then limit the search in _locate_project_name_line to lines
inside that block; change the regex in _locate_project_name_line to allow
optional surrounding single or double quotes around the name value (e.g.
\s*(?P<q>['"]?)(?P<name>...)(?P=q)\s*) while still using re.escape(project_name)
when building the pattern; ensure _find_project_block calls the updated locator
and still returns (start, end, item_indent).
- Around line 594-620: Both helpers currently rebuild lines as "field: value\n"
losing any inline comment/trailing whitespace and forcing LF; update
_update_value and _append_field (and the similar code around the other
occurrence at the same file) to preserve the original line suffix and line
ending: in _update_value keep the original line's trailing content after the
value (including inline comments/whitespace) and reuse its exact EOL when
replacing the line, and in _append_field choose the EOL style by inspecting
neighboring lines (or a provided line's EOL) so inserted lines use the same
CR/LF style and do not drop comments/whitespace semantics when yaml_value is
empty or present.

In `@features/freeze-inplace.feature`:
- Around line 35-68: The selective in-place freeze scenario needs the same "no
.backup" invariant as the all-projects path: after running the command in the
"Only selected project is frozen in-place" scenario, add an assertion that no
".backup" file exists (e.g., no dfetch.yaml.backup or any ".backup" artifact) in
the repo; this ensures the selected_projects(args.projects) code path performs
in-place edits without falling back to backup+rewrite.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c048f3f0-d3b6-4dc0-aa30-5db8c49ac01c

📥 Commits

Reviewing files that changed from the base of the PR and between fa0c753 and 0b6bb85.

📒 Files selected for processing (7)
  • CHANGELOG.rst
  • dfetch/commands/freeze.py
  • dfetch/manifest/manifest.py
  • features/freeze-inplace.feature
  • features/freeze-specific-projects.feature
  • features/steps/manifest_steps.py
  • tests/test_manifest.py

Comment on lines +517 to +569
def _locate_project_name_line(
lines: list[str], project_name: str
) -> tuple[int, int, int, int] | None:
"""Scan *lines* for the ``- name: <project_name>`` entry.

Returns ``(line_idx, item_indent, name_col_start, name_col_end)`` or
``None`` when the project is not found.

- ``line_idx``: 0-based index into *lines*
- ``item_indent``: column of the ``-`` character
- ``name_col_start``: 1-based column of the first character of the name value
(compatible with :class:`ManifestEntryLocation`)
- ``name_col_end``: 0-based exclusive end column of the name value
"""
pattern = re.compile(
r"^(?P<indent>\s*)-\s*name:\s*(?P<name>"
+ re.escape(project_name)
+ r")\s*(?:#.*)?$"
)
for i, line in enumerate(lines):
m = pattern.match(line.rstrip("\n\r"))
if m:
return i, len(m.group("indent")), m.start("name") + 1, m.end("name")
return None


def _find_project_block(lines: list[str], project_name: str) -> tuple[int, int, int]:
"""Return ``(start, end, item_indent)`` for the named project's YAML block.

*start* is the index of the ``- name: <project_name>`` line.
*end* is the exclusive end index (first line that belongs to the next block).
*item_indent* is the column of the ``-`` character.

Raises:
RuntimeError: if the project name is not found.
"""
result = _locate_project_name_line(lines, project_name)
if result is None:
raise RuntimeError(f"Project '{project_name}' not found in manifest text")

start, item_indent, _, _ = result

end = len(lines)
for i in range(start + 1, len(lines)):
line = lines[i]
if not line.strip():
continue
indent = len(line) - len(line.lstrip())
if indent <= item_indent:
end = i
break

return start, end, item_indent
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

Match project names inside projects: only, and handle quoted YAML scalars.

This helper scans the whole file for the first bare - name: match. If a remotes: entry shares a name with a project, _find_project_block() will target the remote block; if the manifest spells the value as name: '176', it will not match at all. That can misplace find_name_in_manifest() results and make in-place freeze rewrite the wrong block or fail on a valid manifest.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dfetch/manifest/manifest.py` around lines 517 - 569, The code currently
matches the first bare "- name: <project>" anywhere in the file and doesn't
accept quoted YAML scalars; update _find_project_block and
_locate_project_name_line so they only search inside the "projects:" list and
accept name values quoted with single or double quotes. Concretely: first scan
for the "projects:" key and compute its block range/indentation, then limit the
search in _locate_project_name_line to lines inside that block; change the regex
in _locate_project_name_line to allow optional surrounding single or double
quotes around the name value (e.g. \s*(?P<q>['"]?)(?P<name>...)(?P=q)\s*) while
still using re.escape(project_name) when building the pattern; ensure
_find_project_block calls the updated locator and still returns (start, end,
item_indent).

Comment on lines +35 to +68
Scenario: Only selected project is frozen in-place
Given a local git repo "superproject2" with the manifest
"""
manifest:
version: '0.0'

projects:
- name: ext/test-repo-tag
url: https://github.com/dfetch-org/test-repo
branch: main

- name: ext/test-repo-tag2
url: https://github.com/dfetch-org/test-repo
branch: main

"""
And all projects are updated in superproject2
When I run "dfetch freeze ext/test-repo-tag" in superproject2
Then the manifest 'dfetch.yaml' in superproject2 is replaced with
"""
manifest:
version: '0.0'

projects:
- name: ext/test-repo-tag
revision: e1fda19a57b873eb8e6ae37780594cbb77b70f1a
url: https://github.com/dfetch-org/test-repo
branch: main

- name: ext/test-repo-tag2
url: https://github.com/dfetch-org/test-repo
branch: main

"""
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Assert the selective in-place path also leaves no .backup.

This scenario exercises a different command path (selected_projects(args.projects)), but only the all-projects scenario checks the no-backup invariant. A regression that falls back to backup+rewrite only for selective freezes would still pass here.

➕ Suggested assertion
     Scenario: Only selected project is frozen in-place
         Given a local git repo "superproject2" with the manifest
             """
             manifest:
               version: '0.0'

               projects:
                 - name: ext/test-repo-tag
                   url: https://github.com/dfetch-org/test-repo
                   branch: main

                 - name: ext/test-repo-tag2
                   url: https://github.com/dfetch-org/test-repo
                   branch: main

             """
         And all projects are updated in superproject2
         When I run "dfetch freeze ext/test-repo-tag" in superproject2
         Then the manifest 'dfetch.yaml' in superproject2 is replaced with
             """
             manifest:
               version: '0.0'

               projects:
                 - name: ext/test-repo-tag
                   revision: e1fda19a57b873eb8e6ae37780594cbb77b70f1a
                   url: https://github.com/dfetch-org/test-repo
                   branch: main

                 - name: ext/test-repo-tag2
                   url: https://github.com/dfetch-org/test-repo
                   branch: main

             """
+        And no file 'dfetch.yaml.backup' exists in superproject2
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@features/freeze-inplace.feature` around lines 35 - 68, The selective in-place
freeze scenario needs the same "no .backup" invariant as the all-projects path:
after running the command in the "Only selected project is frozen in-place"
scenario, add an assertion that no ".backup" file exists (e.g., no
dfetch.yaml.backup or any ".backup" artifact) in the repo; this ensures the
selected_projects(args.projects) code path performs in-place edits without
falling back to backup+rewrite.

- Extract yaml_scalar, find_field, update_value, append_field into the
  new dfetch.util.yaml module so they can be reused outside the manifest
- Fix yaml_scalar: use splitlines()[0] instead of strip() to avoid the
  trailing '\n...' document-end marker that yaml.dump emits for plain strings
- Fix update_value: preserve trailing inline comments (e.g. '# note') when
  replacing a field value, so freeze does not destroy annotations
- manifest.py: replace the four duplicate implementations with aliases that
  delegate to dfetch.util.yaml
- Add BDD scenario: inline comments on fields survive an in-place freeze
- Add unit tests for comment preservation in update_value and
  _update_project_version_in_text

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 2

♻️ Duplicate comments (2)
dfetch/manifest/manifest.py (2)

655-676: ⚠️ Potential issue | 🟠 Major

Stale version fields are not removed during in-place freeze.

_apply_block_updates only adds/updates fields present in fields_to_set but never removes existing revision/tag/branch lines that are no longer in the frozen project. If a user switches pinning mode (e.g., from tag to revision), the old field remains, causing contradictory version state that diverges from the backup+dump path.

🔧 Suggested approach

Before applying updates, remove any existing version fields (revision, tag, branch) that are not in fields_to_set:

def _apply_block_updates(...) -> list[str]:
    # Remove stale version fields first
    fields_to_keep = {f for f, _ in fields_to_set}
    for field in ("revision", "tag", "branch"):
        if field not in fields_to_keep:
            idx = _find_field(block, field, field_indent)
            if idx is not None:
                del block[idx]
    # Then apply updates as before...
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dfetch/manifest/manifest.py` around lines 655 - 676, _in _apply_block_updates
remove stale version fields before inserting/updating: compute fields_to_keep =
{f for f,_ in fields_to_set} and for each of ("revision","tag","branch") if the
field is not in fields_to_keep call _find_field(block, field, field_indent) and
del block[idx] when found, then proceed with the existing insert/update logic
and finally call _set_integrity_hash_in_block as before so stale version lines
are not left behind after a pinning-mode change.

518-541: ⚠️ Potential issue | 🟠 Major

Project name search scope and quoted scalars are not handled.

_locate_project_name_line searches the entire file for - name: <value> and doesn't:

  1. Limit the search to the projects: section — a remotes: entry with the same name would match first
  2. Handle YAML-quoted names like name: 'myproject' or name: "176"

This can cause in-place freeze to target the wrong block or fail on valid manifests with quoted project names.

🔧 Suggested approach

Update the regex to optionally match single/double quotes around the name value:

     pattern = re.compile(
-        r"^(?P<indent>\s*)-\s*name:\s*(?P<name>"
+        r"^(?P<indent>\s*)-\s*name:\s*(?P<quote>['\"]?)(?P<name>"
         + re.escape(project_name)
-        + r")\s*(?:#.*)?$"
+        + r")(?P=quote)\s*(?:#.*)?$"
     )

For scope limitation, consider first locating the projects: key and its indentation, then only searching within that range.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dfetch/manifest/manifest.py` around lines 518 - 541,
_locate_project_name_line currently scans the whole file and uses a regex that
only matches unquoted scalars; update it to first find the projects: mapping
block (locate the "projects:" line and note its indentation, then limit
subsequent search to lines indented under that block until a
same-or-less-indented non-blank non-comment line ends the block) and change the
regex (the pattern variable) to accept optional single or double quotes around
the name value (allowing name: myproj, name: 'myproj', and name: "myproj") while
preserving capture groups for indent and name so the function still returns
(line_idx, item_indent, name_col_start, name_col_end).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 670-671: The code under the "if inserted:" branch constructs lines
with hardcoded "\n" and inserts them directly into the block, bypassing the
existing formatting helper; replace that manual insertion by iterating over the
inserted list and calling the existing _append_field helper for each (f, v) so
formatting/line endings and duplication logic are centralized; ensure you call
_append_field with the current block and the same field_indent/context used
elsewhere (preserve insertion order or insert in reverse if _append_field
mutates the block at the insertion index) and remove the hardcoded string
construction.

In `@dfetch/util/yaml.py`:
- Around line 45-61: The code in update_value only preserves '\n' eol, so CRLF
lines become partially converted; change update_value to detect and preserve
"\r\n" or "\n" (e.g., set eol = "\r\n" if line.endswith("\r\n") elif
line.endswith("\n") else ""), and use that eol when writing back; also update
append_field to choose the EOL for appended lines consistently (for example,
inspect the last line of the block to detect its eol and use that instead of
always appending "\n"), referencing the functions update_value and append_field
so the fix is applied where line endings are constructed.

---

Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 655-676: _in _apply_block_updates remove stale version fields
before inserting/updating: compute fields_to_keep = {f for f,_ in fields_to_set}
and for each of ("revision","tag","branch") if the field is not in
fields_to_keep call _find_field(block, field, field_indent) and del block[idx]
when found, then proceed with the existing insert/update logic and finally call
_set_integrity_hash_in_block as before so stale version lines are not left
behind after a pinning-mode change.
- Around line 518-541: _locate_project_name_line currently scans the whole file
and uses a regex that only matches unquoted scalars; update it to first find the
projects: mapping block (locate the "projects:" line and note its indentation,
then limit subsequent search to lines indented under that block until a
same-or-less-indented non-blank non-comment line ends the block) and change the
regex (the pattern variable) to accept optional single or double quotes around
the name value (allowing name: myproj, name: 'myproj', and name: "myproj") while
preserving capture groups for indent and name so the function still returns
(line_idx, item_indent, name_col_start, name_col_end).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: d0fcc303-cd67-461e-bb8d-0f0a67627fa1

📥 Commits

Reviewing files that changed from the base of the PR and between 0b6bb85 and 0ac98ba.

📒 Files selected for processing (4)
  • dfetch/manifest/manifest.py
  • dfetch/util/yaml.py
  • features/freeze-inplace.feature
  • tests/test_manifest.py

find_field: add an explicit guard to skip lines where the first
non-whitespace character is '#'.  Previously the startswith-prefix check
happened to reject these lines, but the intent was implicit.

_find_project_block: skip commented lines when scanning for the end of a
project block.  A comment at item-indent level (e.g. '  # old pin') was
incorrectly treated as a block boundary, causing fields that followed it
to be excluded from the block and duplicated on freeze instead of updated.

Add unit tests covering:
- find_field skips '# field:' and '#field:' (no space)
- find_field continues past a commented-out duplicate to find the live field
- _find_project_block includes fields that follow a comment at item-indent
- _update_project_version_in_text: commented-out field treated as absent
- _update_project_version_in_text: comment at item-indent does not break block

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

♻️ Duplicate comments (3)
dfetch/manifest/manifest.py (2)

518-556: ⚠️ Potential issue | 🔴 Critical

Restrict project lookup to projects: and accept quoted names.

_locate_project_name_line() still scans the whole document for the first bare - name: match. If a remote shares the same name, _find_project_block() can rewrite the remote block; if the manifest stores the project name as '176' or "176", lookup fails even though the YAML is valid.


657-691: ⚠️ Potential issue | 🟠 Major

Remove version keys that disappeared from project.as_yaml().

This path only updates/adds keys. If a project switches from branch/tag to revision, or loses integrity, the old entries stay in the manifest; when no version keys remain, Lines 690-691 return the original text unchanged. That makes the in-place path diverge from the backup+dump flow and can leave contradictory pins behind.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dfetch/manifest/manifest.py` around lines 657 - 691, The current in-place
path only adds/updates version keys and never removes keys that disappeared from
project.as_yaml(), so stale entries remain; update the logic so when
_collect_version_fields returns no fields and no integrity_hash you remove any
existing version-related lines from the project's block before returning.
Concretely, modify _apply_block_updates (or call it from
_update_project_version_in_text) to detect and delete lines whose field name is
one of the version keys (e.g. "revision", "tag", "branch", "integrity.hash") at
the given field_indent when fields_to_set is empty, and ensure
_set_integrity_hash_in_block is not re-adding; then return the cleaned block so
the in-place flow matches the backup+dump behavior.
dfetch/util/yaml.py (1)

57-64: ⚠️ Potential issue | 🟡 Minor

Preserve the original EOL convention here.

update_value() still collapses \r\n to \n, and append_field() always emits \n. A CRLF-formatted manifest will not round-trip verbatim through these helpers.

Also applies to: 81-83

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dfetch/util/yaml.py` around lines 57 - 64, The code in update_value() (and
similarly in append_field()) always uses "\n" for eol which collapses CRLF;
detect and preserve the original EOL sequence by checking the source line(s) for
"\r\n" vs "\n" (e.g., use line.endswith("\r\n") ? "\r\n" : line.endswith("\n") ?
"\n" : "") or infer the file/ block EOL from existing lines, then use that eol
variable when constructing the replacement string (replace the hard-coded "\n"
in block[line_idx] assignment and in append_field() with the detected eol).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dfetch/manifest/manifest.py`:
- Around line 589-600: The loop that computes sub_end for the nested integrity
mapping incorrectly treats comment-only lines as ending the block, causing
_find_field to miss existing "hash:" entries; update the loop in the manifest
handling (the loop using variables sub_end, block, integrity_line, field_indent)
to ignore comment-only lines (e.g., skip any line where
block[i].strip().startswith("#")) when deciding the end boundary, so _find_field
and subsequent calls to _update_value/_append_field operate over the full nested
mapping.

---

Duplicate comments:
In `@dfetch/manifest/manifest.py`:
- Around line 657-691: The current in-place path only adds/updates version keys
and never removes keys that disappeared from project.as_yaml(), so stale entries
remain; update the logic so when _collect_version_fields returns no fields and
no integrity_hash you remove any existing version-related lines from the
project's block before returning. Concretely, modify _apply_block_updates (or
call it from _update_project_version_in_text) to detect and delete lines whose
field name is one of the version keys (e.g. "revision", "tag", "branch",
"integrity.hash") at the given field_indent when fields_to_set is empty, and
ensure _set_integrity_hash_in_block is not re-adding; then return the cleaned
block so the in-place flow matches the backup+dump behavior.

In `@dfetch/util/yaml.py`:
- Around line 57-64: The code in update_value() (and similarly in
append_field()) always uses "\n" for eol which collapses CRLF; detect and
preserve the original EOL sequence by checking the source line(s) for "\r\n" vs
"\n" (e.g., use line.endswith("\r\n") ? "\r\n" : line.endswith("\n") ? "\n" :
"") or infer the file/ block EOL from existing lines, then use that eol variable
when constructing the replacement string (replace the hard-coded "\n" in
block[line_idx] assignment and in append_field() with the detected eol).
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: c4d4cb95-596e-470b-a040-a219d0c2b6aa

📥 Commits

Reviewing files that changed from the base of the PR and between 0ac98ba and 3049181.

📒 Files selected for processing (3)
  • dfetch/manifest/manifest.py
  • dfetch/util/yaml.py
  • tests/test_manifest.py

integrity sub_end scan skipped comment lines (manifest.py)
  The loop in _update_hash_in_existing_integrity that computes sub_end
  treated comment-only lines as block boundaries, so a '# note' line
  inside an integrity: mapping caused the subsequent hash: line to be
  missed. The loop now skips blank and comment-only lines before
  comparing indentation.

Stale version keys not removed on in-place freeze (manifest.py)
  _apply_block_updates only added/updated fields; keys absent from
  project.as_yaml() (e.g. revision after unfreeze, or branch after
  switching to tag) were left in the manifest as stale entries.
  Added _remove_stale_version_fields and _remove_integrity_block helpers
  that delete version keys not present in the new project data before
  applying updates. Removed the early-return in
  _update_project_version_in_text so cleanup always runs.

CRLF files collapsed to LF (yaml.py)
  update_value always wrote "\n"; append_field also hard-coded "\n".
  update_value now detects "\r\n" vs "\n" from the source line.
  append_field uses a new _detect_eol() helper that inspects the block's
  existing lines to infer the file's EOL convention.

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Sequence[str] for read-only inputs (manifest.py + yaml.py)
  All functions that accept a block/lines parameter but never mutate it
  in-place now declare Sequence[str] instead of list[str]. This resolves
  8 pyright errors in tests where str.splitlines() on a LiteralString
  produced list[LiteralString], which is invariant and could not be
  passed as list[str].

Eliminate duplicated EOL detection (yaml.py)
  _detect_eol and update_value both duplicated the endswith("\r\n") /
  endswith("\n") pattern. Extracted into _line_eol(line) -> str and
  wired both callers through it.

Cleaner copy pattern in _remove_* helpers (manifest.py)
  _remove_integrity_block and _remove_stale_version_fields used
  conditional list() copies. Both now copy unconditionally at the top
  (result = list(block)) for clarity and consistent return type.

https://claude.ai/code/session_01Xd8EcAUkSoJo9YZimGzEuA
Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@dfetch/util/yaml.py`:
- Around line 68-85: The trailing-comment detection in update_value uses
re.search(r"(\s+#.*)$") which can treat hashes inside quoted YAML values as
comments; update update_value to detect an unquoted, whitespace-prefixed '#'
instead of using that regex—scan the stripped line character-by-character
tracking single/double-quote state and locate the first '#' that is not inside
quotes and is preceded by whitespace, then split comment = line[comment_start:]
(or empty if none); keep existing behavior for indent, eol and replacement.
Reference function: update_value.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 197224af-fb0a-4f55-becb-5d5d3ee870a6

📥 Commits

Reviewing files that changed from the base of the PR and between e51247d and f001613.

📒 Files selected for processing (2)
  • dfetch/manifest/manifest.py
  • dfetch/util/yaml.py

Comment on lines +68 to +85
def update_value(
block: Sequence[str], line_idx: int, field_name: str, yaml_value: str
) -> list[str]:
"""Return a copy of *block* with the value on *line_idx* replaced by *yaml_value*.

The indentation, line ending (LF or CRLF), and any trailing comment of
the original line are all preserved so that in-place edits do not destroy
annotations or alter the file's line-ending convention.
"""
result = list(block)
line = result[line_idx]
indent = len(line) - len(line.lstrip())
eol = _line_eol(line)
stripped = line.rstrip("\n\r")
comment_match = re.search(r"(\s+#.*)$", stripped)
comment = comment_match.group(1) if comment_match else ""
result[line_idx] = " " * indent + field_name + ": " + yaml_value + comment + eol
return result
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🧹 Nitpick | 🔵 Trivial

Trailing comment regex may incorrectly match # inside quoted YAML values.

The regex r"(\s+#.*)$" will match # characters that appear within the value portion of a line, not just actual trailing comments. For example, a line like field: "value#with#hash" would have part of the value incorrectly treated as a comment.

However, since this module is used for version fields (revision, tag, branch, hash) which typically don't contain # characters, this is a minor edge case. Consider documenting this limitation or using a more robust YAML-aware approach if needed in the future.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@dfetch/util/yaml.py` around lines 68 - 85, The trailing-comment detection in
update_value uses re.search(r"(\s+#.*)$") which can treat hashes inside quoted
YAML values as comments; update update_value to detect an unquoted,
whitespace-prefixed '#' instead of using that regex—scan the stripped line
character-by-character tracking single/double-quote state and locate the first
'#' that is not inside quotes and is preceded by whitespace, then split comment
= line[comment_start:] (or empty if none); keep existing behavior for indent,
eol and replacement. Reference function: update_value.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants